Repository navigation
test(napi): keep node's stdout in program order in the napi-app fixture - #37199
dylan-conway wants to merge 2 commits into
Conversation
napi.test.ts compares the stdout of `node main.js <test>` and `bun main.js <test>`. On Linux the node side intermittently emitted its console.log lines after all of the addon's printf output, failing `napi_get_value_string_* > behaves like node on edge cases`. - Node's stdout is asynchronous when it is a pipe on POSIX: once a write() returns EAGAIN, libuv queues that console.log and every later one until the event loop runs, while printf keeps writing to fd 1 directly. Make node's stdout blocking in main.js so JS and native output cannot reorder. - The EAGAIN came from BlockingStdoutScope: both runtimes start with stdout unbuffered, and glibc's setvbuf(stdout, NULL, _IOFBF, 8192) keeps the existing 1-byte buffer, so each scope produced thousands of 1–2 byte writes that exhausted the AF_UNIX socketpair's send buffer by skb overhead. Give it a real buffer and restore _IONBF afterwards. No-Verification-Needed: test-fixture-only change
|
@robobun adopt |
|
Adopted. Verifying the fixture change locally and watching CI (build 90588). |
WalkthroughThe NAPI test app now enables blocking POSIX stdout for non-Bun runtimes. ChangesPOSIX stdout ordering
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-fixture-only flake fix, no runtime or assertion changes.
What was reviewed:
main.js:setBlocking(true)is gated to Node with optional chaining, so Bun and Windows paths are unchanged.BlockingStdoutScope:inline staticbuffer is safe — all ~17 call sites are non-nested stack guards on the JS thread; header inclusion in multiple TUs is handled byinline.fflushnow runs beforesetvbuf(required ordering), and the destructor restores_IONBFto match the runtimes' actual startup state rather than_IOLBF.
Extended reasoning...
Overview
This PR fixes a Linux-only flake in test/napi/napi.test.ts where the Node reference run's stdout gets reordered relative to Bun's. Two files are touched, both in the test fixture: test/napi/napi-app/main.js (forces Node's piped stdout to blocking mode) and test/napi/napi-app/utils.h (gives BlockingStdoutScope a real 8 KiB buffer, reorders fflush before setvbuf, and restores _IONBF on exit). No Bun runtime code and no test assertions are changed.
Security risks
None. The changes are confined to a test fixture addon and its JS driver; they alter fd flags and stdio buffering inside a spawned child process.
Level of scrutiny
Low. This is test-infrastructure only, backed by a detailed root-cause analysis (Node's async-pipe stdout + glibc's _shortbuf fallback for setvbuf(NULL, _IOFBF, ...) after _IONBF) with strace evidence. The fix matches the diagnosis and follows the repo guidance for de-flaking: it preserves the property the assertion protects (identical Node/Bun output) rather than weakening the check.
Other factors
I checked all BlockingStdoutScope call sites (one in get_string_tests.cpp, ~16 in standalone_tests.cpp) — every one is a single stack-scoped guard in a synchronous NAPI callback, so the shared inline static buffer never has overlapping users, and inline avoids ODR issues across the multiple TUs that include utils.h. The fflush→setvbuf reordering is a strict correctness improvement per the C standard. The main.js change uses _handle?.setBlocking?.() so it degrades to a no-op where the internal handle is absent. No prior review comments to address.
There was a problem hiding this comment.
LGTM — test-fixture-only de-flake with a clear root cause and strace evidence.
What was reviewed:
main.js:setBlocking(true)is guarded by!process.isBunand optional-chained on both_handleandsetBlocking, so it's a no-op on Bun and on platforms where the API is absent.utils.h:inline static char buffer[8192]is C++17 and ODR-safe across the two TUs that include it; all ~20BlockingStdoutScopeuses are stack-local on the JS thread, never nested/concurrent, so the shared buffer is fine.- Constructor now flushes before
setvbuf(correct order); destructor restores_IONBFwhich matches what both runtimes actually set at startup — more correct than the old_IOLBF.
Extended reasoning...
Overview
This PR touches only the NAPI test fixture (test/napi/napi-app/main.js and test/napi/napi-app/utils.h) to fix a Linux-only flake in napi_get_value_string_* > behaves like node on edge cases (~25% of recent main builds). The flake was on the Node reference side of a checkSameOutput comparison, not in Bun. Two changes: (1) force Node's piped stdout to blocking mode so console.log and native printf cannot reorder after an EAGAIN, and (2) give BlockingStdoutScope a real 8 KiB buffer so glibc doesn't fall back to its 1-byte _shortbuf, and restore _IONBF on exit to match both runtimes' startup state.
Security risks
None. This is test-fixture code that runs a local addon under Node/Bun with piped stdio; no untrusted input, no network, no auth.
Level of scrutiny
Low. No production code is touched, no test assertions are weakened or removed, and the change tightens fixture behavior to match the invariant the test already asserts (identical interleaved output). The PR description includes a precise mechanism (Node's async-pipe stdout + glibc setvbuf shortbuf quirk + AF_UNIX skb accounting), an strace showing the 10 279→61 write reduction and the EAGAIN, and a 640-run stress verification going from 44 reordered to 0.
Other factors
I verified that all ~20 uses of BlockingStdoutScope across get_string_tests.cpp and standalone_tests.cpp are stack-local scopes in synchronous NAPI callbacks on the JS thread — never nested or concurrent — so the single inline static buffer is safe. inline static is C++17 (node-gyp's default on modern Node; MSVC target already uses /std:c++20) and gives one definition across TUs. The main.js line is defensively optional-chained and gated on !process.isBun, so it cannot affect the Bun-side output being compared. The constructor's fflush/setvbuf reorder is the correct sequencing (flush pending unbuffered output before switching modes).
|
Another occurrence of the same failure, for the record: build 97051 ( |
What does this PR do?
Fixes the
napi_get_value_string_* > behaves like node on edge casesflake intest/napi/napi.test.ts(Linux only, ~20 of the last 80 main builds). This is a bug in the test fixture's reference (Node) side, not in Bun.The test runs
main.js test_get_value_stringunder Node and Bun with stdout piped and asserts identical output. The fixture prints from two producers on fd 1: JSconsole.logand the addon'sprintfinsideBlockingStdoutScope. In every failure the diff is the same shape: Bun's output is correctly interleaved, Node's has itsconsole.loglines moved to the very end.Why Node reorders:
write()returnsEAGAINit queues it forEPOLLOUTand every laterconsole.logqueues behind it.test_get_value_stringis fully synchronous, so the queue only drains after all the addon'sprintfoutput has already gone to the fd.EAGAINcomes fromBlockingStdoutScope. Node and Bun bothsetvbuf(stdout, NULL, _IONBF, 0)at startup; on glibc a latersetvbuf(stdout, NULL, _IOFBF, 8192)keeps the existing 1-byte_shortbufinstead of allocating, so each scope emitted its output as thousands of 1–2 bytewrite()s. On Linux Bun's child stdout is anAF_UNIXsocketpair, where each write is its own skb charged ~700 B againstSO_SNDBUF— ~300 undrained tiny writes fill it. The scope's own writes are blocking so they just wait, but the next JS write after the scope restoresO_NONBLOCKgetsEAGAIN.straceof a failing Node run (128 concurrent runs on Linux reproduce it ~7% of the time):Fix: make Node's stdout blocking in
main.js(what Node itself does for TTYs and Windows pipes) so JS and native output cannot reorder, and giveBlockingStdoutScopea real buffer (61 writes instead of 10,279) and restore_IONBFafterwards. Bun already writesconsole.logsynchronously, which is why its side was always ordered. Test expectations are unchanged.How did you verify your code works?
Linux x64: 640 concurrent
node main.js test_get_value_stringruns underBun.spawnwent from 44 reordered to 0;straceshows noEAGAINand 61write(1)calls.bun bd test test/napi/napi.test.tspasses thecheckSameOutputtests on macOS arm64.no test proof · iteration 0 · docs-only change; test-proof not applicable